feat: add GPIO pin interface with embedded-hal-async digital trait support#21
Conversation
|
@EmilNorden Thoughts? |
There was a problem hiding this comment.
Pull request overview
This PR adds a GPIO interface for the PCAL6416A I/O expander driver, providing high-level pin manipulation APIs. The PR builds on top of #20 and increments the version to 0.3.0.
Key changes:
- Introduces a
Pinenum representing the 16 GPIO pins with helper methods for port/bit mapping - Adds pin manipulation methods:
is_pin_high,is_pin_low,set_pin_high,set_pin_low,toggle_pin,is_pin_set_high,is_pin_set_low - Extends device register definitions with input ports and pull-up/down control registers
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 18 comments.
| File | Description |
|---|---|
| src/lib.rs | Adds Pin enum and GPIO manipulation methods to the Device implementation; includes comprehensive test coverage |
| device.yaml | Defines INPUT_PORT0/1, PULL_UP_DOWN_ENABLE_PORT0/1, and PULL_UP_DOWN_SELECT_PORT0/1 registers with field-level documentation |
| Cargo.toml | Bumps version from 0.2.0 to 0.3.0 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
1ea1397 to
15b4bbe
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
15b4bbe to
55e9a1e
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
I think it's a great interface, creating a pin abstraction on top of the registers makes it so much easier to use correctly. |
@EmilNorden Excellent point, the plan is to move toward embedded-hal impl, but I don't want to have a bunch of pins all have mutex protecting the driver instance they all shared. Let me brew on this a bit more. |
Add comprehensive GPIO pin interface with individual pin control: - Implement IoPin type for individual pin operations - Add Device::split() method to create array of 16 IoPin instances - Support both sync and async pin operations (set/read/toggle) - Implement embedded-hal digital traits (InputPin, OutputPin, StatefulOutputPin) - Add full async GPIO methods to Device (set_pin_high_async, etc.) - Implement digital::Error trait for Pcal6416aError - Add comprehensive test coverage for pin operations and embedded-hal trait compliance - Temporarily disable unsafe_code lint to allow UnsafeCell for interior mutability The split() API enables passing individual pins to different functions while maintaining shared access to the underlying I2C device through safe interior mutability patterns.
Improved test coverage for pin operations by testing pins from both ports (port 0 and port 1) in each test case. This ensures the logic is correctly validated across different register addresses since the device has separate registers for each port. Changes: - Updated sync pin tests (is_high, is_low, set_high, set_low, toggle, is_set_high) to test both Pin0 (port 0) and Pin15 (port 1) - Updated async pin tests (is_high_async, is_low_async, set_high_async, set_low_async, toggle_async, is_set_high_async, is_set_low_async) to test both Pin0 and Pin15 - Added inline comments to test expectations for clarity
b36f896 to
fc46286
Compare
Replace the UnsafeCell-based interior mutability pattern with embassy-sync's Mutex for safe concurrent access to individual GPIO pins. Changes: - Add SharedDevice wrapper with Mutex<M, Device> - Update IoPin to use &Mutex instead of &UnsafeCell - Remove synchronous pin methods, keep only async variants - Implement embedded-hal-async traits instead of blocking traits - Update all tests to use SharedDevice and async patterns - Add embassy-sync dependency (0.7.2) - Re-enable unsafe_code lint (was commented out) - Add embedded-hal git patches for latest async traits This provides proper mutex-based synchronization for safe concurrent pin access in async contexts.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@EmilNorden @felipebalbi Please take a look at the async implementation of |
Split the GPIO pin model into separate Port and Pin enums so that each pin is identified by a (Port, Pin) pair instead of a single Pin0-Pin15 value. - Add Port enum with Port0 and Port1 variants - Reduce Pin enum from 16 variants (Pin0-Pin15) to 8 (Pin0-Pin7) - Add port field to IoPin struct alongside existing pin field - Update all Device methods (sync and async) to accept (port: Port, pin: Pin) parameters - Replace if/else port checks with exhaustive match on Port enum - Update SharedDevice::split() to assign correct Port to each pin - Update all tests to use new (Port, Pin) API
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 10 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
`// SAFETY:` is reserved for `unsafe` blocks. These methods use safe async mutex locks.
- Add doc comments to SharedDevice struct and ::new() - Make SharedDevice.device field private
19b3d53 to
c22c561
Compare
Summary
Adds a GPIO pin interface to the PCAL6416A driver, enabling individual pin control through the
embedded-hal-asyncdigital traits (InputPin,OutputPin,StatefulOutputPin). This allows users to pass individual GPIO pins to HAL-generic drivers and async tasks.Built on top of #20.
Changes
New types and API:
Portenum (Port0,Port1) andPinenum (Pin0–Pin7) to model the device's 2×8 GPIO layoutIoPinstruct representing a single GPIO pin with async read/write/toggle operationsSharedDevicewrapper usingembassy_sync::Mutexfor safe concurrent access across pinsSharedDevice::split()method returning an array of 16IoPininstancesembedded-haltrait implementations:embedded_hal::digital::ErrorTypeforIoPinembedded_hal_async::digital::InputPinforIoPinembedded_hal_async::digital::OutputPinforIoPinembedded_hal_async::digital::StatefulOutputPinforIoPinembedded_hal::digital::ErrorforPcal6416aErrorDevice-level pin methods (sync + async):
is_pin_high/is_pin_lowset_pin_high/set_pin_lowis_set_pin_high/is_set_pin_lowtoggle_pinConcurrency model:
UnsafeCell-based interior mutability withembassy_sync::Mutexfor safe shared accessIoPinholds a reference to the shared mutex, so multiple pins can be passed to independent async tasksNew dependencies:
embassy-sync0.7.2embedded-hal/embedded-hal-asyncpatched from git for latest async trait definitionsTests:
embedded-hal-asynctrait compliancetokioUsage example